Use query lib - #823

src/Database/Query.php (1)

62-76: Align cursor helper docblocks with the actual parameter type.

Lines 62 and 73 document Document, but the signature is mixed $value (Lines 65 and 76). Please update phpdoc to match actual behavior.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Query.php` around lines 62 - 76, The PHPDoc for
Query::cursorAfter and Query::cursorBefore incorrectly types the parameter as
Document while the method signatures accept mixed $value; update the `@param`
annotations in both docblocks to "@param mixed $value" (and keep "@return Query"
unchanged) so the docblocks match the actual signatures for the cursorAfter and
cursorBefore methods on the Query class.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

src/Database/Query.php (1)

161-177: ⚠️ Potential issue | 🟠 Major

Fix fallback leakage from limit into offset/cursor state.

Line 167 and Line 176 incorrectly use $limit as fallback, which can corrupt grouped query output when offset/cursor values are missing.

🔧 Proposed fix

                 case self::TYPE_OFFSET:
@@
-                    $offset = $values[0] ?? $limit;
+                    $offset = $values[0] ?? $offset;
                     break;
@@
                 case self::TYPE_CURSOR_AFTER:
                 case self::TYPE_CURSOR_BEFORE:
@@
-                    $cursor = $values[0] ?? $limit;
+                    $cursor = $values[0] ?? $cursor;
                     $cursorDirection = $method === self::TYPE_CURSOR_AFTER ? Database::CURSOR_AFTER : Database::CURSOR_BEFORE;
                     break;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Query.php` around lines 161 - 177, In Query:: (switch handling
TYPE_OFFSET / TYPE_CURSOR_AFTER / TYPE_CURSOR_BEFORE) remove the incorrect
fallback to $limit so $offset and $cursor aren't polluted: instead of assigning
$values[0] ?? $limit, only assign when an explicit value exists (e.g. check
isset($values[0]) or array_key_exists) and otherwise leave $offset/$cursor as
null; keep the first-occurrence guards intact and preserve setting
$cursorDirection when handling TYPE_CURSOR_AFTER vs TYPE_CURSOR_BEFORE
(Database::CURSOR_AFTER / Database::CURSOR_BEFORE).

src/Database/Query.php (1)

53-73: Align cursor helper PHPDoc with the actual mixed parameter type.

Both docblocks still document @param Document $value, but the signatures now accept mixed.

📝 Suggested doc fix

-     * `@param` Document $value
+     * `@param` mixed $value
@@
-     * `@param` Document $value
+     * `@param` mixed $value

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Query.php` around lines 53 - 73, Update the PHPDoc for the Query
helper methods to match the actual parameter type: in the Query class update the
docblocks for cursorAfter and cursorBefore so the `@param` annotation is "@param
mixed $value" (instead of "@param Document $value") and keep the `@return`
annotation as "Query"; ensure these docblocks sit immediately above the
corresponding methods cursorAfter and cursorBefore to maintain accurate
IDE/typehinting.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

src/Database/Query.php (1)

159-169: ⚠️ Potential issue | 🟠 Major

Fix incorrect fallback variable usage for offset and cursor.

Lines 159 and 168 incorrectly fall back to $limit instead of the appropriate variable. This can silently corrupt the grouped query state.

• Line 159: $offset = $values[0] ?? $limit should use $offset (or null)
• Line 168: $cursor = $values[0] ?? $limit should use $cursor (or null)

🔧 Proposed fix

                 case self::TYPE_OFFSET:
                     // Keep the 1st offset encountered and ignore the rest
                     if ($offset !== null) {
                         break;
                     }

-                    $offset = $values[0] ?? $limit;
+                    $offset = $values[0] ?? null;
                     break;
                 case self::TYPE_CURSOR_AFTER:
                 case self::TYPE_CURSOR_BEFORE:
                     // Keep the 1st cursor encountered and ignore the rest
                     if ($cursor !== null) {
                         break;
                     }

-                    $cursor = $values[0] ?? $limit;
+                    $cursor = $values[0] ?? null;
                     $cursorDirection = $method === self::TYPE_CURSOR_AFTER ? Database::CURSOR_AFTER : Database::CURSOR_BEFORE;
                     break;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Query.php` around lines 159 - 169, The switch handling in
Query.php incorrectly falls back to $limit when assigning $offset and $cursor,
corrupting grouped query state; update the assignments in the cases that set
$offset and $cursor (the branches that assign $offset = $values[0] ?? $limit and
$cursor = $values[0] ?? $limit) to use a proper null fallback instead (e.g.
$offset = $values[0] ?? null and $cursor = $values[0] ?? null), keeping the
surrounding logic for cursorDirection (Database::CURSOR_AFTER /
Database::CURSOR_BEFORE) intact.

src/Database/Query.php (1)

54-65: Consider tightening parameter type hint.

The PHPDoc indicates @param Document $value, but the signature uses mixed. Consider using Document as the type hint for better type safety, or update the PHPDoc to reflect the actual accepted types.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Query.php` around lines 54 - 65, The PHPDoc for cursorBefore
indicates the parameter is a Document but the method signature (and cursorAfter)
use mixed; update the signatures to use Document instead of mixed for
cursorBefore and cursorAfter (i.e., change the parameter type from mixed to
Document) to match the PHPDoc and improve type safety, or alternatively update
the PHPDoc to reflect mixed if these methods truly accept other types—adjust the
declarations in the Query class (cursorBefore, cursorAfter) so the docblock and
method signatures are consistent.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

composer.json-64-72 (1)

64-72: ⚠️ Potential issue | 🟠 Major

Path repository will fail in CI environments.

The path repository pointing to ../query is only valid for local development where the sibling query directory exists. In CI environments (GitHub Actions, etc.) and for other contributors cloning this repo, this path won't exist and composer install will fail.

For CI compatibility, either:

1. Publish utopia-php/query to Packagist before merging
2. Use the official VCS repository URL as a fallback

🔧 Proposed fix using VCS repository

     "repositories": [
         {
             "type": "path",
             "url": "../query",
             "options": {
                 "symlink": true
             }
-        }
+        },
+        {
+            "type": "vcs",
+            "url": "https://github.com/utopia-php/query.git"
+        }
     ],

Note: Composer will prefer the path repository if available, falling back to VCS otherwise.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@composer.json` around lines 64 - 72, The current "repositories" entry uses a
path repo ("type": "path", "url": "../query") which will break CI because the
sibling ../query won't exist; update composer.json to remove or guard the
path-only repository and add a fallback VCS repository entry pointing to the
public repo URL for utopia-php/query (or publish to Packagist and use its
package name), so Composer can use the VCS source in CI while still preferring
the local path when present; modify the "repositories" array to include the
official VCS URL (or switch to Packagist) alongside or instead of the existing
path entry.

src/Database/Hook/MongoPermissionFilter.php-26-27 (1)

26-27: ⚠️ Potential issue | 🟠 Major

Escape regex metacharacters in role names to prevent regex injection.

Role names are directly interpolated into the regex pattern without escaping. If a role contains regex metacharacters (e.g., (, ), |, ., *), it could break the regex or enable ReDoS attacks.

🛡️ Proposed fix to escape role names

-        $roles = \implode('|', $this->authorization->getRoles());
+        $roles = \implode('|', \array_map(
+            fn($role) => \preg_quote($role, '/'),
+            $this->authorization->getRoles()
+        ));
         $filters['_permissions']['$in'] = [new Regex("{$forPermission}\\(\".*(?:{$roles}).*\"\\)", 'i')];

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Hook/MongoPermissionFilter.php` around lines 26 - 27, The code
builds a Regex from role names without escaping them, risking regex injection;
update the logic that creates $roles (from $this->authorization->getRoles()) to
escape each role with a regex-escaping function (e.g., preg_quote) before
joining them, then use the escaped string when constructing the Regex used in
the $filters['_permissions']['$in'] assignment (the
Regex("{$forPermission}\\(\".*(?:{$roles}).*\"\\)", 'i') instantiation) so
special characters in role names are neutralized.

src/Database/Traits/Attributes.php-998-1015 (1)

998-1015: ⚠️ Potential issue | 🟠 Major

Rollback the previous default when metadata persistence fails.

If the adapter update succeeds and updateMetadata() then fails, the rollback model recreates the old column without its previous default. The physical schema can remain mutated while metadata rolls back.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Traits/Attributes.php` around lines 998 - 1015, The rollback
Attribute is missing the original default value so if the adapter update
succeeds but updateMetadata() fails the physical column loses its default;
modify the rollback model creation (the Attribute instance assigned to
$rollbackAttrModel) to include the previous default (e.g., $originalDefault) and
ensure whatever variable holds the prior default is captured and passed into the
Attribute constructor, then keep the rollback closure that calls
$this->adapter->updateAttribute($collection, $rollbackAttrModel, $originalKey)
unchanged so the adapter will restore the column including its original default
when metadata persistence fails.

src/Database/Traits/Relationships.php-893-902 (1)

893-902: ⚠️ Potential issue | 🟠 Major

Preserve the original side during rollback.

This recreation hard-codes RelationSide::Parent. If a child-side delete fails after the physical schema change, rollback will restore the relationship on the wrong side.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Traits/Relationships.php` around lines 893 - 902, The rollback
recreates the Relationship with a hard-coded RelationSide::Parent which can
restore the relation on the wrong side; change the code that builds
$recreateRelModel in the rollback to use the original relationship side (e.g.,
the existing side value from the original Relationship instance or the local
$side variable) instead of RelationSide::Parent so the recreated Relationship
preserves the original side; update the constructor call that sets side to use
that original side source (RelationSide value obtained from the original model)
when constructing $recreateRelModel.

src/Database/Traits/Attributes.php-281-284 (1)

281-284: ⚠️ Potential issue | 🟠 Major

Rollback should only delete attributes created in this batch.

$attributeDocuments also contains schema-only orphans that already existed before this call. If one new attribute is created and updateMetadata() fails, cleanupAttributes() will delete those pre-existing columns too.

Also applies to: 317-323

src/Database/Traits/Attributes.php-546-559 (1)

546-559: ⚠️ Potential issue | 🟠 Major

Invalidate both collection and metadata caches after these mutations.

createAttribute(), updateAttribute(), and deleteAttribute() purge cached collection state and the metadata document. updateAttributeMeta() does neither, and renameAttribute() still leaves the metadata document cache warm, so callers can read stale attribute definitions after a successful write.

Also applies to: 1284-1292

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Traits/Attributes.php` around lines 546 - 559, After performing
attribute-mutating operations (specifically in updateAttributeMeta() and
renameAttribute()), ensure you clear both the collection cache and the metadata
document cache just like createAttribute()/updateAttribute()/deleteAttribute()
do: call the same cache invalidation logic used elsewhere after the
$this->updateMetadata(...) call (and before returning), and also ensure any
cache keys for the metadata document are purged so callers don't read stale
attribute definitions; reference updateAttributeMeta(), renameAttribute(), and
the existing $this->updateMetadata(...) / self::EVENT_ATTRIBUTE_UPDATE flow to
locate the correct spot to add the invalidation.

src/Database/Adapter.php-685-697 (1)

685-697: ⚠️ Potential issue | 🟠 Major

Don’t report unsupported relationship operations as success.

Traits\Relationships uses these return values to decide whether to update metadata. Returning true here lets an adapter without a real implementation claim success while leaving the physical schema unchanged.

🧱 Proposed fix

 public function createRelationship(Relationship $relationship): bool
 {
-    return true;
+    return false;
 }
 
 public function updateRelationship(Relationship $relationship, ?string $newKey = null, ?string $newTwoWayKey = null): bool
 {
-    return true;
+    return false;
 }
 
 public function deleteRelationship(Relationship $relationship): bool
 {
-    return true;
+    return false;
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Adapter.php` around lines 685 - 697, The Adapter currently
reports success for unsupported relationship operations; update the methods
createRelationship(Relationship $relationship), updateRelationship(Relationship
$relationship, ?string $newKey = null, ?string $newTwoWayKey = null) and
deleteRelationship(Relationship $relationship) to not claim success — return
false (or throw a clear UnsupportedOperationException) instead of returning true
so Traits\Relationships won't assume metadata was updated; ensure the change is
applied to those method bodies and keep the signatures unchanged.

src/Database/Traits/Relationships.php-518-579 (1)

518-579: ⚠️ Potential issue | 🟠 Major

Update both relationship metadata documents atomically.

These updateAttributeMeta() calls commit independently. If the first collection update succeeds and the second one fails, the catch only reverts the adapter rename; it never restores the already-persisted metadata change on the first collection.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Traits/Relationships.php` around lines 518 - 579, The metadata
updates via updateAttributeMeta on the primary collection, the related
collection (oldTwoWayKey), and the junction (when ManyToMany) must be performed
atomically; currently if one update fails the others remain persisted. Fix by
either wrapping all metadata updates and the adapter rename in a single DB
transaction (begin/commit/rollback) so any failure rolls back all changes, or if
transactions are not available, capture the existing attribute state for
collection->getId()/$id, relatedCollection->getId()/oldTwoWayKey, and junction
entries (if RelationType::ManyToMany) before applying updates and, inside the
catch, call updateAttributeMeta to restore those saved states (and ensure
purgeCachedCollection is similarly reverted). Update logic references:
updateAttributeMeta, getJunctionCollection, withRetries->purgeCachedCollection,
and the adapter->updateRelationship/Relationship model so both metadata and
adapter rename are consistent.

src/Database/Traits/Relationships.php-143-157 (1)

143-157: ⚠️ Potential issue | 🟠 Major

Check the related collection for twoWayKey collisions before creating the relationship.

The duplicate scan only inspects the source collection. checkAttribute() only enforces limits/width, so an existing attribute on the related collection with the same $twoWayKey is not rejected before partial relationship creation starts.

Also applies to: 191-192

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Traits/Relationships.php` around lines 143 - 157, The
duplicate-check currently inspects only the source collection attributes (loop
over $attributes) and misses collisions on the related collection; update the
logic in the Relationships trait where you check for existing relationship
attributes (the foreach that compares ColumnType::Relationship->value, twoWayKey
and relatedCollection id) to also fetch and iterate the related collection's
attributes (use $relatedCollection->getAttribute('attributes', [])) and compare
their options['twoWayKey'] case-insensitively to $twoWayKey and their
relatedCollection id to the source collection id, throwing
DuplicateException('Related attribute already exists') on conflict; apply the
same symmetric check to the other similar block referenced by checkAttribute()
(the block around the other relationship-creation path) so both creation flows
validate twoWayKey collisions on the opposite collection before proceeding.

src/Database/Traits/Attributes.php-572-576 (1)

572-576: ⚠️ Potential issue | 🟠 Major

Keep the metadata-only helpers behind the same invariants as updateAttribute().

updateAttributeRequired() can mark an attribute required while leaving its old default in place, updateAttributeFilters() can remove mandatory filters like datetime, and updateAttributeDefault() never checks vector length against the configured dimension count. These helpers can persist attribute metadata that the full update path would reject.

Also applies to: 627-631, 644-653

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Traits/Attributes.php` around lines 572 - 576, The three
metadata-only helpers (updateAttributeRequired, updateAttributeFilters,
updateAttributeDefault) bypass the same validation/invariants enforced by
updateAttribute and can persist invalid metadata; change each helper to call the
full update path or reuse its validation logic by delegating to updateAttribute
(or to the common validation function used by updateAttribute) instead of
directly mutating via updateAttributeMeta: ensure updateAttributeRequired
validates compatibility between required flag and the existing default,
updateAttributeFilters preserves/enforces mandatory filters (e.g., datetime) and
validates any removed filters against attribute type, and updateAttributeDefault
enforces vector length vs configured dimension count and any type-specific
constraints before persisting. Ensure you reference and reuse updateAttribute or
its shared validators so all invariants are consistently applied.

src/Database/Hook/PermissionWrite.php-131-136 (1)

131-136: ⚠️ Potential issue | 🟠 Major

Route permission reads/deletes through WriteContext::execute.

The insert path uses the context executor, but these branches call execute() directly and some of the delete helpers ignore the boolean result entirely. A failed permission mutation can therefore leave <collection>_perms out of sync with the document write.

Also applies to: 187-192, 208-215, 227-231, 262-266

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Hook/PermissionWrite.php` around lines 131 - 136, The permission
delete branches in PermissionWrite (use of ($context->newBuilder),
$removeBuilder->delete(), and the subsequent ($context->executeResult) returning
$deleteStmt) call $deleteStmt->execute() directly and ignore the WriteContext
executor and result; change these to run deletes through the WriteContext
executor (use WriteContext::execute on the prepared statement returned by
($context->executeResult) with Database::EVENT_PERMISSIONS_DELETE), check the
boolean return value and propagate/handle failures (throw or abort the parent
write) so permission mutations cannot silently fail; apply the same fix pattern
to the other affected blocks that build/delete perms (the sections analogous to
lines 187-192, 208-215, 227-231, 262-266).

src/Database/Traits/Relationships.php-493-515 (1)

493-515: ⚠️ Potential issue | 🟠 Major

Don’t treat the unchanged source key as proof that the rename already happened.

When only $newTwoWayKey changes, $actualNewKey still points at the current source column. The orphan-recovery fallback will then mark $adapterUpdated = true as soon as it sees that existing column, even if the related/junction rename never happened.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Traits/Relationships.php` around lines 493 - 515, The recovery
logic in Relationships.php incorrectly treats finding $actualNewKey in schema as
proof the rename completed; instead, when catching the Throwable you must verify
the rename truly occurred by checking that the new column exists AND the
original source column was removed (or that the two-way/junction column change
also exists) — use getSchemaAttributes(), $this->adapter->filter($actualNewKey)
and also compare against the filtered original/source key (e.g., filteredOldKey)
and/or the filtered $newTwoWayKey to ensure the found column isn't simply the
unchanged source; only set $adapterUpdated = true if the new name exists AND the
old name does not (or the corresponding two-way column was updated), otherwise
rethrow the DatabaseException (preserving $e as previous).

src/Database/Traits/Documents.php-970-975 (1)

970-975: ⚠️ Potential issue | 🟠 Major

Use the hook's returned document in bulk updates.

updateDocument() reassigns afterDocumentUpdate(), but updateDocuments() ignores the returned value. Any relationship hook that rewrites the payload will be skipped on this path.

🔧 Proposed fix

                     $hook = $this->relationshipHook;
                     if ($hook?->isEnabled()) {
-                        $this->silent(fn () => $hook->afterDocumentUpdate($collection, $document, $new));
+                        $new = $this->silent(fn () => $hook->afterDocumentUpdate($collection, $document, $new));
                     }
 
                     $document = $new;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Traits/Documents.php` around lines 970 - 975, The bulk-update
path is ignoring the relationship hook's return value: in the block using
$this->relationshipHook and calling $hook->afterDocumentUpdate($collection,
$document, $new), capture and use the hook's returned document (replace $new
with the hook return when non-null) before assigning to $document so any payload
rewrites by afterDocumentUpdate are preserved; update the logic around
relationshipHook/isEnabled and the assignment to $document to use the hook's
return value (from afterDocumentUpdate) instead of discarding it.

src/Database/Traits/Documents.php-401-409 (1)

401-409: ⚠️ Potential issue | 🟠 Major

Run castingAfter() on single-document creates even when hooks are off.

This path only normalizes adapter output inside the relationship-hook branch. MariaDB::createDocument() returns the pre-cast document instance, so a single create can return DB-shaped values while createDocuments() returns normalized ones.

🔧 Proposed fix

         $hook = $this->relationshipHook;
         if ($hook !== null && !$hook->isInBatchPopulation() && $hook->isEnabled()) {
             $fetchDepth = $hook->getWriteStackCount();
             $documents = $this->silent(fn () => $hook->populateDocuments([$document], $collection, $fetchDepth));
-            $document = $this->adapter->castingAfter($collection, $documents[0]);
+            $document = $documents[0];
         }
 
+        $document = $this->adapter->castingAfter($collection, $document);
         $document = $this->casting($collection, $document);
         $document = $this->decode($collection, $document);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Traits/Documents.php` around lines 401 - 409, The current code
only runs $this->adapter->castingAfter(...) when $this->relationshipHook is
present and enabled, causing single-document creates to return DB-shaped values;
move or duplicate the adapter post-processing so castingAfter is applied for
single-document flows regardless of the relationship hook state: ensure that
after the potential hook population block you call
$this->adapter->castingAfter($collection, $document) (using the same normalized
$documents[0] result when hook ran) before running $this->casting(...) and
$this->decode(...), so both createDocument() and createDocuments() return
consistently normalized documents; use the existing symbols relationshipHook,
populateDocuments, castingAfter, casting, decode and silent to implement this
change.

src/Database/Traits/Documents.php-274-301 (1)

274-301: ⚠️ Potential issue | 🟠 Major

Preserve relationship roots when pruning selected fields.

$attributesToKeep only records the literal selector. A query like select(['author.name']) will therefore remove the populated author attribute itself, and aliased projections are dropped for the same reason. Please retain the root segment of dotted paths, plus any projection alias, before removing attributes.

Based on learnings: Repo utopia-php/database: Relationship selects are always evaluated in the main alias context (no per-collection aliasing). In Utopia\Database\Database::applySelectFiltersToDocuments, do not rely on Query::getAlias() for relationships; instead, preserve the root of dotted paths and any projection alias (Query::getAs()) when filtering attributes.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Traits/Documents.php` around lines 274 - 301, The pruning logic
in applySelectFiltersToDocuments builds $attributesToKeep from selectQueries by
using literal selectors only, which drops relationship root attributes and
aliased projections (e.g., select(['author.name']) or projections with
Query::getAs()). Update the loop that populates $attributesToKeep (the foreach
over $selectQueries and getValues()) to also: for each selector string preserve
its root segment (the part before the first dot) and, if Query::getAs() is
present for that query, preserve the alias name as well; do not rely on
Query::getAlias() for relationship selects. Ensure these root keys and
projection aliases are added to $attributesToKeep before the
wildcard/internal-key checks so removeAttribute only strips truly unselected
fields.

src/Database/Traits/Documents.php-1174-1215 (1)

1174-1215: ⚠️ Potential issue | 🟠 Major

Don't drop brand-new empty documents as a no-op.

When $old is empty and the incoming document has no user fields/operators, $hasChanges stays false and the item is removed from the batch. upsertDocument() then falls back to getDocument() and returns an empty document instead of creating a valid empty record.

🔧 Proposed fix

-            if (!$hasChanges) {
+            if (!$hasChanges && !$old->isEmpty()) {
                 // If not updating a single attribute and the document is the same as the old one, skip it
                 unset($documents[$key]);
                 continue;
             }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Traits/Documents.php` around lines 1174 - 1215, The current
change-detection logic in the Documents trait can wrongly treat brand-new empty
incoming docs as unchanged and drop them from $documents[$key], because when
$old is empty and regularUpdatesUserOnly and $operators are empty $hasChanges
remains false; update the logic in the block that computes $hasChanges (the
checks that reference $operators, $attribute, $skipPermissionsUpdate, $old,
regularUpdatesUserOnly, self::INTERNAL_ATTRIBUTES) to explicitly mark a document
as changed when $old is null/empty (new record) even if no user fields/operators
are present so upsertDocument() will create the empty record instead of falling
back to getDocument(); ensure this new case still respects internal attribute
filtering and the subsequent unset($documents[$key]) skip only applies to truly
identical existing documents.

src/Database/Adapter/MariaDB.php-603-609 (1)

603-609: ⚠️ Potential issue | 🟠 Major

Use side-aware junction naming when renaming many-to-many columns.

deleteRelationship() computes the junction table name differently for parent vs child sides, but updateRelationship() always uses <collection>_<related>. Child-side renames will target the wrong junction table.

🔧 Proposed fix

-                $junctionName = '_' . $collection->getSequence() . '_' . $relatedCollection->getSequence();
+                $junctionName = $side === RelationSide::Parent
+                    ? '_' . $collection->getSequence() . '_' . $relatedCollection->getSequence()
+                    : '_' . $relatedCollection->getSequence() . '_' . $collection->getSequence();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Adapter/MariaDB.php` around lines 603 - 609,
updateRelationship() is using a fixed junction name ('_' .
$collection->getSequence() . '_' . $relatedCollection->getSequence()) which
ignores the relationship side and causes child-side renames to target the wrong
junction table; change the junction name computation in updateRelationship() to
mirror the logic used in deleteRelationship() (i.e., compute $junctionName based
on the $side or whichever variable deleteRelationship() uses to decide order of
$collection->getSequence() and $relatedCollection->getSequence()), then use that
side-aware $junctionName when calling $renameCol for $key/$newKey and
$twoWay/$newTwoWayKey so renames operate on the correct many-to-many table.

src/Database/Adapter/MariaDB.php-1211-1228 (1)

1211-1228: ⚠️ Potential issue | 🟠 Major

Add handlers for TYPE_COVERS, TYPE_NOT_COVERS, TYPE_SPATIAL_EQUALS, and TYPE_NOT_SPATIAL_EQUALS in the spatial query match statement.

The validator in src/Database/Validator/Queries.php allows these four new spatial query types, but the match statement in src/Database/Adapter/MariaDB.php (lines 1211-1228) does not handle them. These queries will throw Unknown spatial query method at runtime when executed. The same issue exists in src/Database/Adapter/Postgres.php.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Adapter/MariaDB.php` around lines 1211 - 1228, The match in
MariaDB.php handling spatial query methods misses Query::TYPE_COVERS,
Query::TYPE_NOT_COVERS, Query::TYPE_SPATIAL_EQUALS and
Query::TYPE_NOT_SPATIAL_EQUALS; add match arms that map TYPE_COVERS to
"ST_Covers({$alias}.{$attribute}, {$geom})", TYPE_NOT_COVERS to "NOT
ST_Covers(...)", TYPE_SPATIAL_EQUALS to "ST_SpatialEquals({$alias}.{$attribute},
{$geom})" and TYPE_NOT_SPATIAL_EQUALS to "NOT ST_SpatialEquals(...)" (use the
same pattern/templating as the existing arms such as Query::TYPE_CONTAINS and
Query::TYPE_EQUAL), and apply the identical changes to the corresponding match
in Postgres.php so both adapters support the four new Query types.

src/Database/Adapter/Mongo.php-1064-1069 (1)

1064-1069: ⚠️ Potential issue | 🟠 Major

Resolve partial-index types by attribute name, not loop offset.

Line 1165 builds $indexAttributeTypes as [$attrName => $type], but Line 1067 reads $indexAttributeTypes[$i]. That falls back to string for numeric/date fields and can exclude the real field type from the partial index entirely.

Suggested fix

         if (in_array($type, [IndexType::Unique, IndexType::Key])) {
             $partialFilter = [];
             foreach ($attributes as $i => $attr) {
-                $attrType = $indexAttributeTypes[$i] ?? ColumnType::String->value; // Default to string if type not provided
+                $sourceAttribute = $index->attributes[$i] ?? $attr;
+                $attrType = $indexAttributeTypes[$sourceAttribute] ?? ColumnType::String->value;
                 $attrType = $this->getMongoTypeCode($attrType);
                 $partialFilter[$attr] = ['$exists' => true, '$type' => $attrType];
             }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Adapter/Mongo.php` around lines 1064 - 1069, The partial index
builder in Mongo.php is using the loop index to look up attribute types but
$indexAttributeTypes is keyed by attribute name; update the lookup in the loop
that constructs $partialFilter (in the block handling IndexType::Unique and
IndexType::Key) to use the attribute name ($attr) as the key instead of $i, e.g.
fetch the type with $indexAttributeTypes[$attr] ?? ColumnType::String->value and
then pass that into $this->getMongoTypeCode so the correct field type is used
for the partial index filter.

docker-compose.yml-6-7 (1)

6-7: ⚠️ Potential issue | 🟡 Minor

Document local development setup requirement for sibling query repository.

The volume mount ../query/src:/usr/src/code/vendor/utopia-php/query/src and build context .. assume a specific local directory structure where the query repository exists as a sibling. This dependency is not documented in CONTRIBUTING.md. While the volume mount failure is non-fatal in CI (the container starts regardless), local development with docker compose up -d --build will fail without the sibling query repository, making setup unclear for new contributors.

Also applies to: 21-21

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docker-compose.yml` around lines 6 - 7, The docker-compose configuration uses
a sibling repo via the volume mount
'../query/src:/usr/src/code/vendor/utopia-php/query/src' and a build context
'..' (with dockerfile database/Dockerfile), which must be documented; update
CONTRIBUTING.md to state that the `query` repository must exist as a sibling (or
provide steps to clone it) before running `docker compose up -d --build`, note
the mount is optional for CI and point to the alternative (remove/disable the
volume or use a docker-compose.override.yml) for contributors who cannot have
the sibling repo, and mirror this documentation for the other occurrence
referenced (line 21) so local dev setup is explicit.

README.md-636-638 (1)

636-638: ⚠️ Potential issue | 🟡 Minor

Add import statement for ForeignKeyAction in the example.

The example shows ForeignKeyAction::Cascade->value but doesn't include the necessary use statement. This could confuse developers trying to use the code.

📝 Suggested addition before line 636

use Utopia\Database\ForeignKeyAction;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@README.md` around lines 636 - 638, Add a use/import for the ForeignKeyAction
enum in the example so the symbols like ForeignKeyAction::Cascade->value,
ForeignKeyAction::SetNull->value, and ForeignKeyAction::Restrict->value resolve;
specifically, add the statement to import the class (e.g., use
Utopia\Database\ForeignKeyAction;) near the top of the snippet where the example
code begins so ForeignKeyAction is available in that example context.

README.md-758-776 (1)

758-776: ⚠️ Potential issue | 🟡 Minor

Add import statement for SetType in the example.

Similar to ForeignKeyAction, the SetType enum usage examples should include the import statement for clarity.

📝 Suggested addition before line 758

use Utopia\Database\SetType;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@README.md` around lines 758 - 776, Add the missing import for the SetType
enum used in the README examples by inserting the statement importing SetType
(e.g., use Utopia\Database\SetType;) near the other example imports so the
usages of SetType::Assign / SetType::Append / SetType::Prepend in the
setAttribute examples resolve; ensure it appears alongside existing imports like
Permission and Role so the sample snippet is self-contained and clear.

src/Database/Relationship.php-21-31 (1)

21-31: ⚠️ Potential issue | 🟡 Minor

toDocument() omits collection and key properties.

The toDocument() method doesn't serialize $this->collection or $this->key, but fromDocument() accepts both as inputs (collection as parameter, key from attribute). This asymmetry may cause data loss during round-trip serialization if these fields need to be preserved.

If this is intentional (because these fields come from external context), consider documenting this behavior.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Relationship.php` around lines 21 - 31, The toDocument() method
on Relationship currently omits $this->collection and $this->key while
fromDocument() (and the static fromDocument method) accept collection and key,
causing asymmetrical serialization; modify Relationship::toDocument() to include
'collection' => $this->collection and 'key' => $this->key in the returned
Document (or, if omission is intentional, add a clear docblock on
Relationship::toDocument() and fromDocument() explaining that collection and key
are provided externally and therefore not persisted) so round-trip serialization
is consistent.

src/Database/Hook/RelationshipHandler.php-243-243 (1)

243-243: ⚠️ Potential issue | 🟡 Minor

Unused loop variable $index.

The $index variable from the foreach loop is declared but never used. This was correctly flagged by static analysis.

🔧 Proposed fix

-        foreach ($relationships as $index => $relationship) {
+        foreach ($relationships as $relationship) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Hook/RelationshipHandler.php` at line 243, The foreach in
RelationshipHandler.php declares an unused loop variable $index; change the loop
signature from foreach ($relationships as $index => $relationship) to foreach
($relationships as $relationship) (or use foreach ($relationships as $_ =>
$relationship) if you prefer an explicit unused key) so the unused $index is
removed, keeping the loop body using $relationship unchanged.

src/Database/Hook/RelationshipHandler.php-1803-1814 (1)

1803-1814: ⚠️ Potential issue | 🟡 Minor

Same shadowing issue: $document in deleteCascade.

The loop variable $document shadows the method parameter, same issue as in deleteSetNull. The loop iterates over junction documents while the original parameter is the document being deleted.

🔧 Rename loop variable

-                foreach ($junctions as $document) {
+                foreach ($junctions as $junctionDoc) {
                     if ($side === RelationSide::Parent->value) {
                         $this->db->deleteDocument(
                             $relatedCollection->getId(),
-                            $document->getAttribute($key)
+                            $junctionDoc->getAttribute($key)
                         );
                     }
                     $this->db->deleteDocument(
                         $junction,
-                        $document->getId()
+                        $junctionDoc->getId()
                     );
                 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Hook/RelationshipHandler.php` around lines 1803 - 1814, In
deleteCascade, the foreach loop uses $document which shadows the method
parameter $document; rename the loop variable (e.g., to $junctionDocument or
$junctionDoc) wherever it's used within the loop so the parameter $document
remains the original document being deleted, and update the two deleteDocument
calls to reference the renamed loop variable
($junctionDocument->getAttribute($key) and $junctionDocument->getId()) while
leaving the method parameter untouched.

src/Database/Attribute.php-76-92 (1)

76-92: ⚠️ Potential issue | 🟡 Minor

fromArray() omits status and options fields.

Unlike fromDocument(), the fromArray() method doesn't extract status and options from the input array. This asymmetry could cause data loss when attributes with these fields are serialized via toDocument() and then reconstructed via fromArray().

🔧 Proposed fix to include status and options

     public static function fromArray(array $data): self
     {
         $type = $data['type'] ?? 'string';
 
         return new self(
             key: $data['$id'] ?? $data['key'] ?? '',
             type: $type instanceof ColumnType ? $type : ColumnType::from($type),
             size: $data['size'] ?? 0,
             required: $data['required'] ?? false,
             default: $data['default'] ?? null,
             signed: $data['signed'] ?? true,
             array: $data['array'] ?? false,
             format: $data['format'] ?? null,
             formatOptions: $data['formatOptions'] ?? [],
             filters: $data['filters'] ?? [],
+            status: $data['status'] ?? null,
+            options: $data['options'] ?? null,
         );
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Attribute.php` around lines 76 - 92, fromArray() currently
ignores the 'status' and 'options' keys so reconstructing an Attribute via
Attribute::fromArray loses data present in toDocument()/fromDocument(); update
Attribute::fromArray to read 'status' and 'options' from the input array (e.g.
$data['status'] ?? <sensible default> and $data['options'] ?? <default empty
array>) and pass them into the Attribute constructor (matching how
fromDocument() does), ensuring defaults mirror fromDocument/toDocument behavior.

src/Database/Hook/RelationshipHandler.php-272-283 (1)

272-283: ⚠️ Potential issue | 🟡 Minor

Loose equality comparison may cause unexpected behavior.

Using == for comparing $oldValue and $value (Line 272) can lead to unexpected type coercion, especially when comparing Documents, arrays, or mixed types. This could cause relationship updates to be incorrectly skipped.

🔧 Consider strict comparison or dedicated comparison logic

-            if ($oldValue == $value) {
+            if ($this->areRelationshipValuesEqual($oldValue, $value)) {

Alternatively, if loose comparison is intentional for this specific use case, add a comment explaining why.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Hook/RelationshipHandler.php` around lines 272 - 283, The loose
comparison using $oldValue == $value in RelationshipHandler (around the block
handling RelationType and RelationSide) can produce incorrect skips; change this
to strict comparison or explicit comparison logic: use === for scalar checks, if
both values are Document instances compare their IDs (e.g., $oldValue->getId()
=== $value->getId()), and for arrays use a deep equality check (or
serialize/sort then compare) before deciding to set or remove attributes via
$document->setAttribute/$document->removeAttribute; if loose comparison was
intentional, replace the == with a clear comment explaining why and what cases
rely on coercion.

src/Database/Hook/RelationshipHandler.php-1727-1732 (1)

1727-1732: ⚠️ Potential issue | 🟡 Minor

Parameter shadowing: $document reused in loop.

The foreach loop variable $document shadows the method parameter $document (from Line 1640), which could cause confusion or unintended behavior if the original document is needed after the loop.

🔧 Rename loop variable

-                foreach ($junctions as $document) {
-                    $this->db->skipRelationships(fn () => $this->db->deleteDocument(
-                        $junction,
-                        $document->getId()
-                    ));
+                foreach ($junctions as $junctionDoc) {
+                    $this->db->skipRelationships(fn () => $this->db->deleteDocument(
+                        $junction,
+                        $junctionDoc->getId()
+                    ));
                 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Hook/RelationshipHandler.php` around lines 1727 - 1732, The
foreach loop reuses the variable name $document which shadows the method
parameter $document; rename the loop variable (e.g., $junctionDocument or
$junctionItem) and update its usage inside the loop so the method parameter
remains available unchanged, keeping the existing calls to
$this->db->skipRelationships(fn () => $this->db->deleteDocument($junction,
<loop-var>->getId())); ensure only the loop variable name is changed (no other
logic altered).

src/Database/Traits/Indexes.php-359-366 (1)

359-366: ⚠️ Potential issue | 🟡 Minor

Fix rollback index TTL default: should be 0, not 1, to match TTL semantics elsewhere.

The rollback index uses getAttribute('ttl', 1) as default, but throughout the codebase TTL semantics treat 0 (or missing attribute) as "no TTL configured". In Documents.php:247, the code explicitly checks if ($ttlSeconds <= 0 || !$ttlAttr) to determine whether to skip TTL processing. Additionally, the Validator/Index.php:810 uses getAttribute('ttl', 0) as default. Using 1 as the default means if the original index had no TTL attribute, the rollback would incorrectly create an index with a 1-second TTL, causing documents to expire prematurely. Change the default to 0 to preserve the "no TTL" state when the attribute is missing.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Traits/Indexes.php` around lines 359 - 366, The rollback index
creation uses getAttribute('ttl', 1) which incorrectly sets a 1s TTL when the
attribute is missing; change the default to 0 so $rollbackIndex is created with
no TTL when absent by updating the call on $indexDeleted->getAttribute('ttl', 1)
to use 0 (the code building the Index instance with IndexType::from(...) and
attributes/lengths/orders should remain the same) to match Validator/Index.php
and Documents.php TTL semantics.

src/Database/Adapter/SQLite.php-43-70 (1)

43-70: ⚠️ Potential issue | 🟡 Minor

SQLite advertises it does not support Capability::Upserts but still implements upsert functionality.

Line 62 removes Capability::Upserts from the advertised capabilities, yet SQLite inherits upsertDocuments() from SQL and overrides executeUpsertBatch() to handle SQLite's ON CONFLICT syntax. This creates an inconsistency: the capability advertisement does not match the runtime behavior. While this doesn't currently cause dead code (no code gate upsert calls on supports(Capability::Upserts)), the mismatch violates the contract that removed capabilities should not be implemented. Either re-add Capability::Upserts to SQLite's capabilities, or remove the executeUpsertBatch() override if SQLite should not support upserts.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Adapter/SQLite.php` around lines 43 - 70, The SQLite adapter
currently removes Capability::Upserts in capabilities() while still implementing
upsertDocuments() and overriding executeUpsertBatch(), causing a contract
mismatch; fix by either (A) re-adding support: remove Capability::Upserts from
the $remove array in capabilities() so parent::capabilities() reports Upserts,
or (B) disable runtime support: delete or revert the
SQLite::executeUpsertBatch() override (and any SQLite-specific upsert helpers)
so the adapter no longer implements upsert behavior — choose one approach and
make the changes consistently (references: capabilities(), Capability::Upserts,
upsertDocuments(), executeUpsertBatch(), parent::capabilities()).

src/Database/Adapter/Mongo.php-1081-1081 (1)

1081-1081: ⚠️ Potential issue | 🟡 Minor

Remove the ->value accessor to fix the unreachable readiness loop.

At line 1081, $type is an IndexType enum case (assigned from $index->type at line 44), but the comparison $type === IndexType::Unique->value compares an enum case object to its backing scalar value. This will never match in PHP—enum cases and their backing values are distinct types. As a result, the readiness loop is unreachable and the code never waits for unique indexes to be fully built.

The fix is to compare the enum case directly:

Fix

-            if ($type === IndexType::Unique->value) {
+            if ($type === IndexType::Unique) {

This is consistent with all other enum comparisons in the same function (lines 1020, 1042, 1050, 1055, 1060), which use enum cases without the ->value accessor.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Adapter/Mongo.php` at line 1081, The readiness loop is
unreachable because it compares the enum case object $type to the backing scalar
of IndexType::Unique (using ->value); locate the comparison of $type and
IndexType::Unique in the readiness/wait loop and remove the ->value accessor so
the code compares the enum case directly (i.e., use IndexType::Unique) to match
how other enum checks in this function handle $type.

src/Database/Adapter/Pool.php (1)

80-88: Use the short class name for consistency.

Line 80 uses the fully qualified class name \Utopia\Database\Capability even though Capability is already imported at line 7. Use the short name for consistency with the rest of the file.

Suggested fix

-    public function supports(\Utopia\Database\Capability $feature): bool
+    public function supports(Capability $feature): bool
     {
         return $this->delegate(__FUNCTION__, \func_get_args());
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Adapter/Pool.php` around lines 80 - 88, Change the
fully-qualified parameter type in the supports method to the short imported
class name: replace "\Utopia\Database\Capability $feature" with "Capability
$feature" in the supports method signature so it matches the rest of the file
and uses the imported Capability symbol; ensure the method still calls
$this->delegate(__FUNCTION__, func_get_args()) unchanged.

src/Database/Adapter/SQLite.php (1)

247-248: Remove the duplicated _perms index creation.

These same two indexes are already created on Lines 247-248. Repeating them here just adds extra catalog probes and will become observable if index-create hooks/events are added later.

♻️ Proposed cleanup

-            $this->createIndex("{$id}_perms", new Index(key: '_index_1', type: IndexType::Unique, attributes: ['_document', '_type', '_permission']));
-            $this->createIndex("{$id}_perms", new Index(key: '_index_2', type: IndexType::Key, attributes: ['_permission', '_type']));

Also applies to: 265-266

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Database/Adapter/SQLite.php` around lines 247 - 248, The code calls
$this->createIndex twice for the same "{$id}_perms" table with identical
Index(...) definitions (using Index(key: '_index_1', type: IndexType::Unique,
attributes: ['_document','_type','_permission']) and Index(key: '_index_2',
type: IndexType::Key, attributes: ['_permission','_type'])), causing duplicate
index creation; remove the duplicated createIndex calls (the repeated
"{$id}_perms" invocations) so each index is created only once, leaving a single
call to createIndex for each Index definition (references: createIndex, Index,
IndexType).